Add redundant type annotations lint #10570

AlessioC31 · 2023-03-30T13:16:18Z

Hello, I'm trying to add the redundat_type_annotations lint.

It's still WIP but I'd like to start gathering some feedbacks to be sure that I'm not doing things 100% wrong :)

Right now it still misses lints like:

I've some problems regarding the second example above, in the init part of the Local I have:

init: Some(
                Expr {
                    hir_id: HirId(DefId(0:24 ~ playground[e1bd]::main).58),
                    kind: Call(
                        Expr {
                            hir_id: HirId(DefId(0:24 ~ playground[e1bd]::main).59),
                            kind: Path(
                                TypeRelative(
                                    Ty {
                                        hir_id: HirId(DefId(0:24 ~ playground[e1bd]::main).61),
                                        kind: Path(
                                            Resolved(
                                                None,
                                                Path {
                                                    span: src/main.rs:77:21: 77:27 (#0),
                                                    res: Def(
                                                        Struct,
                                                        DefId(0:17 ~ playground[e1bd]::STest2),
                                                    ),
                                                    segments: [
                                                        PathSegment {
                                                            ident: STest2#0,
                                                            hir_id: HirId(DefId(0:24 ~ playground[e1bd]::main).60),
                                                            res: Def(
                                                                Struct,
                                                                DefId(0:17 ~ playground[e1bd]::STest2),
                                                            ),
                                                            args: None,
                                                            infer_args: true,
                                                        },
                                                    ],
                                                },
                                            ),
                                        ),
                                        span: src/main.rs:77:21: 77:27 (#0),
                                    },
                                    PathSegment {
                                        ident: get_numb#0,
                                        hir_id: HirId(DefId(0:24 ~ playground[e1bd]::main).62),
                                        res: Err,
                                        args: None,
                                        infer_args: true,
                                    },
                                ),
                            ),
                            span: src/main.rs:77:21: 77:37 (#0),
                        },
                        [],
                    ),
                    span: src/main.rs:77:21: 77:39 (#0),
                },
            ),

And I'm not sure how to get the return type of the function STest2::func() since the resolved path DefId points to the struct itself and not the function. Do you have any idea on how I could get this information in this case?

Thanks!

changelog: changelog: [redundant_type_annotations]: New lint to warn on redundant type annotations

fixes #9155

rustbot · 2023-03-30T13:16:24Z

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @xFrednet (or someone else) soon.

Please see the contribution instructions for more information. Namely, in order to ensure the minimum review times lag, PR authors and assigned reviewers should ensure that the review label (S-waiting-on-review and S-waiting-on-author) stays updated, invoking these commands when appropriate:

@rustbot author: the review is finished, PR author should check the comments and take action accordingly
@rustbot review: the author is ready for a review, this PR will be queued again in the reviewer's queue

bors · 2023-04-01T12:58:48Z

☔ The latest upstream changes (presumably #10534) made this pull request unmergeable. Please resolve the merge conflicts.

xFrednet

Welcome to Clippy. Thank you for your patience while waiting on a review!

I've left some review comments. Please don't be discouraged by them, I believe this lint can become quite complex depending on how detailed and how many edge cases we want to catch. Please reach out if you require any further assistance. :)

clippy_lints/src/redundant_type_annotations.rs

tests/ui/redundant_type_annotations.rs

clippy_lints/src/redundant_type_annotations.rs

AlessioC31 · 2023-04-04T21:45:25Z

Welcome to Clippy. Thank you for your patience while waiting on a review!

I've left some review comments. Please don't be discouraged by them, I believe this lint can become quite complex depending on how detailed and how many edge cases we want to catch. Please reach out if you require any further assistance. :)

Thanks for the review!

xFrednet · 2023-04-06T19:18:02Z

Thanks for the review!

You're welcome. Are you still working on the implementation or is the branch ready for another review? :)

AlessioC31 · 2023-04-06T22:10:02Z

Thanks for the review!

You're welcome. Are you still working on the implementation or is the branch ready for another review? :)

I'm still working on it. Right now I've problems with Lit type of expressions (like how to map a Lit to a Ty) :)
Do you have any suggestion on where I could have a look?

Or when is a type annotation useful in case of a Lit?

pub enum LitKind {
    /// A string literal (`"foo"`). The symbol is unescaped, and so may differ
    /// from the original token's symbol.
    Str(Symbol, StrStyle),
    /// A byte string (`b"foo"`). Not stored as a symbol because it might be
    /// non-utf8, and symbols only allow utf8 strings.
    ByteStr(Lrc<[u8]>, StrStyle),
    /// A byte char (`b'f'`).
    Byte(u8),
    /// A character literal (`'a'`).
    Char(char),
    /// An integer literal (`1`).
    Int(u128, LitIntType),
    /// A float literal (`1.0`, `1f64` or `1E10f64`). The pre-suffix part is
    /// stored as a symbol rather than `f64` so that `LitKind` can impl `Eq`
    /// and `Hash`.
    Float(Symbol, LitFloatType),
    /// A boolean literal (`true`, `false`).
    Bool(bool),
    /// Placeholder for a literal that wasn't well-formed in some way.
    Err,
}

In almost all these cases we can say that the annotation is redundant, or am I missing anything?
The only exception I can think about is in case of unsuffixed numbers (Int and Float literals)

bors · 2023-04-07T07:54:52Z

☔ The latest upstream changes (presumably #10601) made this pull request unmergeable. Please resolve the merge conflicts.

xFrednet · 2023-04-07T11:05:00Z

I'm still working on it. Right now I've problems with Lit type of expressions (like how to map a Lit to a Ty) :) Do you have any suggestion on where I could have a look?

I don't think there is an easy way to map it. You could request the type of the expression, but that will probably not help too much.

Or when is a type annotation useful in case of a Lit?

A type annotation is useful or even required when the literal is unsuffixed. For example 8 can be several types, the compiler would then look at its usages to determine the type. If the user writes let x: u8 = 8, the type should be kept IMO. However, if the literal has a suffix 8_u8 then the type can be removed. You can take a look at LitKind::is_unsuffixed() for this one. For &str etc it should be fine to remove the type as the literal is clear.

xFrednet

Generally speaking, I think this is a good start, which we could actually merge as a minimum viable product. There are still several examples that could be improved. However, most of them seem a bit complicated to fix and the question is how much benefit we'll get from them. It should be fine as is, since all ToDos are false negatives.

What are your thoughts? Do you want to try perfecting it in this PR or get this merge ready and create follow-ups? (Assuming, you're still interested in the lint)

tests/ui/redundant_type_annotations.rs

xFrednet · 2023-04-21T15:55:54Z

tests/ui/redundant_type_annotations.rs

+
+    let _return: u32 = new_pie.return_an_int();
+
+    let _return: String = String::from("test");


This currently doesn't trigger a lint, even if the function comment says it should. I'm not quite sure why, as String::new() etc are covered.

Wow, I missed that, thanks!

I digged into it a bit, it turns out it is related to generics.
When I do cx.tcx.type_of(func_defid) of the function, I get EarlyBinder(fn(T) -> Self {<Self as std::convert::From<T>>::from})

If I then execute subst_identity or skip_binder on this, I get fn(T) -> Self {<Self as std::convert::From<T>>::from}

Then, when I do func_ty.fn_sig(cx.tcx).output() where func_ty is the type I wrote previously, I get Binder(Self, []), then I interpret the return type as Self instead of String.

Do you know what I could use in this case? I tried having a look at other lints but I was not able to find anything, thanks!

Hey @xFrednet , do you have any clue on this? Thanks!

Sorry, I missed your question earlier 😅. I'm not totally sure about this one, but you might want to take a look at:

type_dependent_def to resolve the method target

Maybe qpath_res to resolve Self?

If both of these don't work, I'd probably ask on Zulip the others might have a good idea for this one :)

Sorry, I missed your question earlier 😅. I'm not totally sure about this one, but you might want to take a look at:

type_dependent_def to resolve the method target

Maybe qpath_res to resolve Self?

If both of these don't work, I'd probably ask on Zulip the others might have a good idea for this one :)

I've tried the two methods you suggested without any luck. I've asked on Zulip, let's see.

Thanks!

AlessioC31 · 2023-04-21T16:01:55Z

Generally speaking, I think this is a good start, which we could actually merge as a minimum viable product. There are still several examples that could be improved. However, most of them seem a bit complicated to fix and the question is how much benefit we'll get from them. It should be fine as is, since all ToDos are false negatives.

What are your thoughts? Do you want to try perfecting it in this PR or get this merge ready and create follow-ups? (Assuming, you're still interested in the lint)

Hello @xFrednet , thanks again.

I'm still interested and I plan to perfecting this PR (at least adding support for primitive types and the suggestions you mentioned in the previous reviews).
I had to pause working on this because I had a busy period, it should be better next week!

AlessioC31 · 2023-05-16T14:30:21Z

Hey @AlessioC31, what is the current status of the PR? Are there still open TODOs, besides the rebase? :)

Hello @xFrednet , there's still the problem with the String::from (and other generics since it's the same I guess). I've asked on Zulip but I haven't received any answer, maybe I can wrap this PR, I'll add support for refs, rebase and then it's ready to being reviewed. It won't lint in case generics are used but maybe I can come back at this when I understand how to do it

AlessioC31 · 2023-05-19T15:53:24Z

Hello @xFrednet, I think this is ready for review.

We miss a couple of things which I think they could be added in the future, like:

Support for generics
Refs returned from anything else than a MethodCall (maybe it's because of my little experience with rust yet, but I think this shouldn't happen often, what do you think?)
Complex types (tuples, arrays, etc...)
Path to something different than a primitive type. Like for now this lint supports cases like let a: u32 = u32::MAX but it doesn't support things like let a: MyStruct = Somewhere::MyStaticInstance. Do you think we need to add this?

Thanks!

xFrednet · 2023-05-21T17:45:26Z

Hello @xFrednet, I think this is ready for review.

We miss a couple of things which I think they could be added in the future, like:

Support for generics

As long as we don't have false positives and only false negatives, that's totally fine :). Could you add this limitation, as well as the complex types and path one into the lint documentation? I like to use a ### Limitations section to list such things. After this PR, we should ideally also create issues for these :)

Refs returned from anything else than a MethodCall (maybe it's because of my little experience with rust yet, but I think this shouldn't happen often, what do you think?)

We should make sure that normal function calls are also detected. For one, because they're used quite often and because it should be a simple addition. I can mark the required changes in the next review :)

Complex types (tuples, arrays, etc...)

Complex types are sadly complex to lint. I've been contemplating, how to best handle lints for these, for a different project. I sadly haven't found a good option. This is to say, that it's fine if these are not caught, probably. I guess that Clippy has quite a few lints that can't handle these :)

Path to something different than a primitive type. Like for now this lint supports cases like let a: u32 = u32::MAX but it doesn't support things like let a: MyStruct = Somewhere::MyStaticInstance. Do you think we need to add this?

I believe that should be fine. Such a pattern is rarely used in the projects I'm using. We can leave this as an improvement for the future :)

Thanks!

You're very welcome, thank you for working on Clippy! I'll review this PR in the upcoming week :)

bors · 2023-05-23T22:11:09Z

☔ The latest upstream changes (presumably #10810) made this pull request unmergeable. Please resolve the merge conflicts.

xFrednet

This version looks good to me. I would say, it only requires the limitations section and a rebase. Then we can merge it! :)

clippy_lints/src/redundant_type_annotations.rs

AlessioC31 · 2023-05-29T09:27:47Z

I think I've addressed all your comments and rebased to master. Should I squash the commits?

Thanks!

xFrednet · 2023-06-02T17:21:39Z

I think I've addressed all your comments and rebased to master.

There still seems to be a conflict, could you resolve that one?

Should I squash the commits?

It's not mandatory, but would be nice, 1 - 5 commits would be a good balance :)

I intend to review this over the weekend :)

AlessioC31 · 2023-06-04T04:47:59Z

I think I've addressed all your comments and rebased to master.

There still seems to be a conflict, could you resolve that one?

Should I squash the commits?

It's not mandatory, but would be nice, 1 - 5 commits would be a good balance :)

I intend to review this over the weekend :)

Ok I've rebased to master and reduced the amount of commits, thanks!

AlessioC31 · 2023-06-05T08:23:01Z

Rebased again :D

xFrednet · 2023-06-06T21:10:22Z

This version looks good to me. Thank you for all the work you put into this lint! I appreciate it!

@bors r+

bors · 2023-06-06T21:10:24Z

📌 Commit a9b468f has been approved by xFrednet

It is now in the queue for this repository.

bors · 2023-06-06T21:10:32Z

⌛ Testing commit a9b468f with merge cc8ead2...

bors · 2023-06-06T21:26:24Z

☀️ Test successful - checks-action_dev_test, checks-action_remark_test, checks-action_test
Approved by: xFrednet
Pushing cc8ead2 to master...

rustbot assigned xFrednet Mar 30, 2023

rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties label Mar 30, 2023

AlessioC31 force-pushed the redundant_type_annotations branch 5 times, most recently from ddc455a to 4770ee1 Compare March 30, 2023 13:53

xFrednet reviewed Apr 4, 2023

View reviewed changes

AlessioC31 force-pushed the redundant_type_annotations branch 2 times, most recently from fffd85b to 5ae56f0 Compare April 4, 2023 21:43

AlessioC31 force-pushed the redundant_type_annotations branch 8 times, most recently from e858255 to 5977676 Compare April 6, 2023 08:25

AlessioC31 force-pushed the redundant_type_annotations branch 2 times, most recently from 9b60cbf to a29fd4f Compare April 6, 2023 23:16

AlessioC31 force-pushed the redundant_type_annotations branch from a29fd4f to b0405c7 Compare April 17, 2023 17:55

xFrednet reviewed Apr 21, 2023

View reviewed changes

AlessioC31 force-pushed the redundant_type_annotations branch from db33978 to 0583a29 Compare May 19, 2023 15:47

AlessioC31 force-pushed the redundant_type_annotations branch 2 times, most recently from be50a5f to 1ef6023 Compare May 19, 2023 16:08

AlessioC31 requested a review from xFrednet May 20, 2023 08:29

xFrednet reviewed May 26, 2023

View reviewed changes

clippy_lints/src/redundant_type_annotations.rs Show resolved Hide resolved

clippy_lints/src/redundant_type_annotations.rs Show resolved Hide resolved

AlessioC31 force-pushed the redundant_type_annotations branch from ac43a4c to c427a17 Compare May 29, 2023 09:26

AlessioC31 force-pushed the redundant_type_annotations branch from c427a17 to a31df5c Compare May 29, 2023 09:28

AlessioC31 requested a review from xFrednet May 31, 2023 18:33

AlessioC31 force-pushed the redundant_type_annotations branch 2 times, most recently from 5d4cee7 to 3df2a43 Compare June 4, 2023 04:47

AlessioC31 added 5 commits June 5, 2023 10:22

Add redundant type annotations lint

01b1057

Move redundant_type_annotations to restriction

6776608

Extract common logic to function

6f26df1

Add support to returned refs from MethodCall

29ab954

Add Limitations section

a9b468f

AlessioC31 force-pushed the redundant_type_annotations branch from 3df2a43 to a9b468f Compare June 5, 2023 08:22

xFrednet approved these changes Jun 6, 2023

View reviewed changes

bors merged commit cc8ead2 into rust-lang:master Jun 6, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add redundant type annotations lint #10570

Add redundant type annotations lint #10570

AlessioC31 commented Mar 30, 2023 •

edited

Loading

rustbot commented Mar 30, 2023

bors commented Apr 1, 2023

xFrednet left a comment

AlessioC31 commented Apr 4, 2023

xFrednet commented Apr 6, 2023

AlessioC31 commented Apr 6, 2023 •

edited

Loading

bors commented Apr 7, 2023

xFrednet commented Apr 7, 2023

xFrednet left a comment

xFrednet Apr 21, 2023

AlessioC31 Apr 24, 2023

AlessioC31 May 3, 2023 •

edited

Loading

xFrednet May 3, 2023

AlessioC31 May 8, 2023

AlessioC31 commented Apr 21, 2023

AlessioC31 commented May 16, 2023 •

edited

Loading

AlessioC31 commented May 19, 2023 •

edited

Loading

xFrednet commented May 21, 2023 •

edited

Loading

bors commented May 23, 2023

xFrednet left a comment

AlessioC31 commented May 29, 2023

xFrednet commented Jun 2, 2023

AlessioC31 commented Jun 4, 2023

AlessioC31 commented Jun 5, 2023

xFrednet commented Jun 6, 2023

bors commented Jun 6, 2023

bors commented Jun 6, 2023

bors commented Jun 6, 2023


		let _return: u32 = new_pie.return_an_int();

		let _return: String = String::from("test");

Add redundant type annotations lint #10570

Add redundant type annotations lint #10570

Conversation

AlessioC31 commented Mar 30, 2023 • edited Loading

rustbot commented Mar 30, 2023

bors commented Apr 1, 2023

xFrednet left a comment

Choose a reason for hiding this comment

AlessioC31 commented Apr 4, 2023

xFrednet commented Apr 6, 2023

AlessioC31 commented Apr 6, 2023 • edited Loading

bors commented Apr 7, 2023

xFrednet commented Apr 7, 2023

xFrednet left a comment

Choose a reason for hiding this comment

xFrednet Apr 21, 2023

Choose a reason for hiding this comment

AlessioC31 Apr 24, 2023

Choose a reason for hiding this comment

AlessioC31 May 3, 2023 • edited Loading

Choose a reason for hiding this comment

xFrednet May 3, 2023

Choose a reason for hiding this comment

AlessioC31 May 8, 2023

Choose a reason for hiding this comment

AlessioC31 commented Apr 21, 2023

AlessioC31 commented May 16, 2023 • edited Loading

AlessioC31 commented May 19, 2023 • edited Loading

xFrednet commented May 21, 2023 • edited Loading

bors commented May 23, 2023

xFrednet left a comment

Choose a reason for hiding this comment

AlessioC31 commented May 29, 2023

xFrednet commented Jun 2, 2023

AlessioC31 commented Jun 4, 2023

AlessioC31 commented Jun 5, 2023

xFrednet commented Jun 6, 2023

bors commented Jun 6, 2023

bors commented Jun 6, 2023

bors commented Jun 6, 2023

AlessioC31 commented Mar 30, 2023 •

edited

Loading

AlessioC31 commented Apr 6, 2023 •

edited

Loading

AlessioC31 May 3, 2023 •

edited

Loading

AlessioC31 commented May 16, 2023 •

edited

Loading

AlessioC31 commented May 19, 2023 •

edited

Loading

xFrednet commented May 21, 2023 •

edited

Loading